fix(recovery): persist audit recovery claims - #2167
Conversation
postgres.AuditTransactionStore only overrode WriteDB, GetSchema and CreateSchema, so it inherited ClaimPendingTransactions and ReleaseRecoveryClaim from sqlcommon. The common implementations are a plain SELECT and a no-op, neither of which persists a claim, so every replica selected the same pending audit transactions on every tick and processed all of them. The audit store now uses the same atomic UPDATE ... RETURNING claim the owner store has had since it was introduced. The SQL is identical for the two stores and only the requests table differs, so it moves to a shared recoveryClaimStore that both hold as a field. It is a named field rather than an embedded one because sqlcommon.TransactionStore also provides these methods, and embedding both at the same depth makes the selectors ambiguous, which silently drops them from the method set. No schema change. recovery_claimed_by, recovery_claim_expires_at and the two supporting indexes already come from the shared sqlcommon schema that audit storage uses. CleanupExpiredClaims is not exposed on the audit store. Nothing calls it on either path and the claim query already reclaims expired leases inline. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
|
@adecaro , @AkramBitar , let me know your thoughts on this , whenever you have chance , thanks :) |
|
Thanks for the fix. Based on my understanding the root cause is clear and the approach is correct. The audit store was inheriting a no-op claim path from One concern I'd like to understand better: what happens when processing takes longer than the lease duration? Once a replica claims a transaction and starts processing it, if the work exceeds Could you clarify:
If the work is idempotent, this is acceptable as-is and worth a comment saying so. If it's not, the lease expiry window is a correctness gap that should be addressed before or shortly after this lands. In addition, does is the last thing that we need to do for the issue #2143 cna we attach it to this PR and close one we close this PR? |
|
Thanks @AkramBitar, fair thing to press on. Short version: the lease isn't what provides mutual exclusion here, leadership is.
Caveat I should have put in the PR description: that doesn't hold on the audit path yet. This PR leaves 1. Idempotent? Mostly, and I'd rather be specific than claim it wholesale. 2. Renewal? None today. Precedent if we want one: 3. Config? Defaults 30s lease / 30s TTL / 5s scan / batch 100 / 4 workers. 4. #2143 — not the last piece, so I'd rather it not auto-close. The leadership half is written and tested but sits on #2085, which changes the leader factory signature to bind the lock id at construction; doing it on main means inventing a scheme #2085 deletes. There's also a trap: giving audit the owner's lock id makes the two managers contend so one never sweeps, which is why it's derived per store. Preference is to link this as partial and let the leadership PR carry |
|
@atharrva01 thanks a lot for the response. So this PR should go after PR #2085 (i.e., wait for this one to be merged first)? |
|
yessssss @AkramBitar |
3 participants
Fixes the second half of #2143, the one that causes the duplicated work.
postgres.AuditTransactionStoreonly overrodeWriteDB,GetSchemaandCreateSchema, so itinherited
ClaimPendingTransactionsandReleaseRecoveryClaimfromsqlcommon. Those are aplain
SELECTand a no-op, so no claim is ever persisted and every replica selects the samepending audit transactions on every tick and processes all of them. The audit store now uses the
same atomic
UPDATE ... RETURNINGclaim the owner store has had since it was introduced.The SQL is identical for the two stores and only the requests table differs, so it moves into a
shared
recoveryClaimStorethat both hold as a field. It is a named field and not an embeddedone on purpose:
sqlcommon.TransactionStoreprovides the same two methods, and embedding both atthe same depth makes the selectors ambiguous, which drops them from the method set and quietly
stops the store satisfying
driver.AuditTransactionStore. Explicit forwarding keeps the overridevisible where it is called.
No schema change.
recovery_claimed_by,recovery_claim_expires_atand the two supportingindexes already come from the shared
sqlcommonschema that audit storage uses.Left for a follow-up
The other half of #2143, leadership always being granted on the audit path, is not fixed here.
That needs a lock id derived per store, which is exactly what #2085 introduces, and doing it on
top of
mainwould mean inventing a second scheme that competes with the one already in review.Worth noting that both recovery managers currently read the same
AdvisoryLockIDfrom config, sohanding audit a real factory today would make owner and audit recovery contend on one lock and
starve each other.
This is still worth landing on its own. With the atomic claim in place, each pending audit
transaction goes to exactly one replica even while every replica believes it is the leader, so
the duplicated processing is gone. What remains is N cheap claim queries per tick instead of one.
Question
CleanupExpiredClaimsis not exposed on the audit store. Nothing calls it on either path, andthe claim query already reclaims expired leases inline through
recovery_claim_expires_at < NOW(),so adding it looked like new dead code. Happy to add it for symmetry if you would rather have it.
Tests
Five new tests in
audit_recovery_claim_test.gomirroring the owner ones: claim exclusivityacross two replicas, lease expiry, release making a row immediately available again, release
under the wrong owner being ignored, and one pinning that audit and owner claims stay in their
own requests tables. The existing owner tests pass unchanged,
ageRequestsjust takes the claimstore now so both suites can share it.